Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow user to select an explicit currency for dividends #1915

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chrisaut
Copy link

Allows the user to select a different currency for dividend payments, different from either security currency and/or account currency.

image

Example use case:
US stocks bought (and tracked) in EUR at a German exchange, but Dividend gets paid in USD.

I'm not sure if this is done yet, putting it up in the hope for a quick review if the approach is acceptable.

Some Forum discussion

@chrisaut chrisaut force-pushed the feat/explicitDividendCurrency branch from 31fd967 to a9d382e Compare December 29, 2020 11:10
@chirlu
Copy link
Member

chirlu commented Dec 29, 2020

The custom drop-down menu doesn’t fit on my screen, and there is no way to scroll. If it is to stay at this place, it should be an actual drop-down (showing only the ISO codes), like when changing the currency of an account. But I would prefer a separate line with a label like “payment currency” or “dividend currency”. This would also provide enough space for a full currency drop-down, like in the dialogue for editing a security.

Another remark: There are lots of lines with dangling spaces or tabs at the end.

@chrisaut
Copy link
Author

The custom drop-down menu doesn’t fit on my screen, and there is no way to scroll. If it is to stay at this place, it should be an actual drop-down (showing only the ISO codes), like when changing the currency of an account.

Well it's the same dropdown as under statement of assets. I don't particularly like it either (see #1876 ) but I still think it's better then the currency selection dropdown style IMO because at least it has the option of seperators for previously used currencies. I guess we could extend the other dropdowns with that too though. If other's disagree I can change it though, I'm not stuck on it.

I assume you mean this style?
image

There is also this currency selection:
image

I kinda think we should unify currency selection though, create a reusable component that looks the same everywhere and is easier to use. Try switching from EUR to USD quickly in the dropdown under accounts for example, it's a usability nightmare. Don't think this should be part of this though.

But I would prefer a separate line with a label like “payment currency” or “dividend currency”. This would also provide enough space for a full currency drop-down, like in the dialogue for editing a security.

I can put it on a new line, just thought it would waste space. Do others agree?

Another remark: There are lots of lines with dangling spaces or tabs at the end.

Yeah good point, will clean it up with next commit.

@buchen
Copy link
Member

buchen commented Dec 30, 2020

I would also propose to separate the two things:

  • First: allow to record dividends in a currency other than the security currency.
  • Second: have a better usable drop-down for selecting currencies

I propose to make this PR about the first topic.

Allowing other currencies makes sense. Particularly because the currency determines both: the currency of the security prices downloaded and the currency of the dividend statements. But one wants to be able to separates this: trade a security on a EUR exchange and still receive USD dividends.

The change of the UI, however, is only one part. I do not know for sure, but so far in many calculations I assume that the currency fits to the currency of the security. To merge the change, I would like to see a test case (similar to what you find in the n.a.p.tests/src/scenarios folder) that tests ClientSnapshot, ClientPerformanceSnapshot, and SecurityPerformanceSnapshot procure the results you expect.

I can put it on a new line, just thought it would waste space. Do others agree?

I think most of the time users will not touch the currency. I kind of like it right next to the "per share" item.

@chirlu
Copy link
Member

chirlu commented Dec 30, 2020

Well it's the same dropdown as under statement of assets.

Oh, indeed, that one is unusable in the same way. :-) (But I never use it, so I never noticed.) I‘ve just created issue #1920 for that.

one wants to be able to separates this: trade a security on a EUR exchange and still receive USD dividends.

As an aside, I own some shares that used to pay dividends in DEM once upon a time and now pay in EUR. So that is a use case, too.

@chrisaut
Copy link
Author

I would like to see a test case (similar to what you find in the n.a.p.tests/src/scenarios folder) that tests ClientSnapshot, ClientPerformanceSnapshot, and SecurityPerformanceSnapshot procure the results you expect.

Ok good, I will add tests and see if things break

I can put it on a new line, just thought it would waste space. Do others agree?

I think most of the time users will not touch the currency. I kind of like it right next to the "per share" item.

Ok, let's leave it where it is then

@tomasherman
Copy link

Hey everyone, i was wondering it this PR is going to be worked on further or merged :) Maybe one thing to consider would be adding Dividend currency as a paramter of Security (with default set to currency used to denominate the currency). So that the user can set it once when creating the security and then the dialog pics the current currency. What do you think?

@chrisaut
Copy link
Author

chrisaut commented Jun 23, 2021

I do still want this feature, very much so in fact, nearly all my dividends come from US but paid in EUR. If I remember correctly I got a bit stuck trying to setup a test context that would be able to exercise all the new cases. Then I unfortunately got sidetracked and busy. From the manual testing I had done it did seem to work though.

So, perhaps someone can take this as a starting point, or inspiration and finish it. If not I may pick it up sometime over the summer, but I just can't promise when. I can close the PR if you prefer.

@tomasherman
Copy link

i wanted to pick this up but having a really rough time with eclipse (intellij guy here :D) ... it keeps crashing on me, im not sure i will be able to get at it properly, it drives me crazy

@bbatsov
Copy link
Contributor

bbatsov commented Jun 25, 2021

Is the project built using Eclipse RCP? Haven't done any Java in ages, but I remember this was the last reason I had to use Eclipse for anything. :D

@ah4c
Copy link
Contributor

ah4c commented Jun 1, 2022

I was curious so I copied the branch from chrisaut's repo and rebased it on buchen's master. There was one tiny merge conflict that was easy to resolve. I am not sure that I have a use case for this as I prefer to import my transactions from the bank's PDFs.

If someone wants to pick this up but is worried about rebasing over so many commits then you could take my branch. https://github.com/ah4c/portfolio/tree/feat/explicitDividendCurrency

git remote add ah4c [email protected]:ah4c/portfolio.git
git fetch ah4c
git checkout ah4c/feat/explicitDividendCurrency

@chrisaut chrisaut force-pushed the feat/explicitDividendCurrency branch from a9d382e to 262be4c Compare August 2, 2023 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants